🤖 Claude Code Review

🐛 Bugs

• require_timing_partition tolerance is too tight. max(1e-12, total * 1e-9) gives a 1-nanosecond-relative tolerance for a seconds-valued total_s. For a 1-second run the tolerance is 1 ns, so floating-point accumulation across many timings entries will routinely trigger the "refuses to clamp" error on legitimate data. The multiplicative factor should be something like 1e-6 (1 ppm), not 1e-9.

• _build_stall_phase always computes data_stall + gpu_active + sync from the same timings list, but require_timing_partition is called with the per-frame-summed values. If any individual frame's _t_total_s is zero or missing, sum(t["_t_total_s"] ...) is 0, and require_positive_duration inside the partition helper will raise—but the outer elapsed check has already passed, so the error is confusing and the user can't distinguish "wall clock fine, per-frame totals broken" from "overall timing broken."

• run_profile: sar_output file handle leak on early return. The open(bundle.host_sar, "w") is assigned to sar_output and closed at the bottom, but if workload.wait() raises (e.g. KeyboardInterrupt) the file is never closed. Use a try/finally or contextlib.ExitStack.

• _cmd_edge_manifest empty-manifest check is after return 0 in original but before it in the diff—actually the check is correct, but rows is only populated if write_manifest succeeded; verify write_manifest doesn't silently swallow errors before the check.

• optimize_af2 validates warmup >= 0 but never uses warmup in the visible diff fragment. If warmup iterations are supposed to be excluded from the _fold_all timing, there's no evidence they are, meaning cold-cache latency pollutes the A/B measurement.

• **classify_bottleneck: sc_score can be None when _serialized_fraction returns None, but mem_score >= 1.0 and (sc_score is None or mem_score > sc_score) evaluates correctly—however the fall-through if sc_score is None: return UNCLASSIFIED is reached even when mem_score >= 1.0, because the first branch already returned. The logic is correct but the ordering makes it non-obvious; a reader could miss that the UNCLASSIFIED branch is only reached when mem_score < 1.0 and sc_score is None.

🔒 Security

• run_profile passes tools.py_spy, tools.sar, and tools.nsys directly into subprocess.Popen list-form commands. If ProfilerTools.detect() sources these paths from environment variables or config files without sanitization, an attacker controlling those paths can execute arbitrary binaries. Validate that discovered tool paths are absolute and within expected locations (e.g. /usr/bin, /usr/local/bin).

⚡ Performance

• aggregate in baseline.py iterates runs four times (values, gpu_values, provenance loop, gates). For typical bench sizes this is fine, but the provenance loop could be merged with the existing runs iteration.

• _fold_all calls torch.cuda.synchronize() only when torch.cuda.is_available(), which is correct, but the timer (time.perf_counter()) is stopped after the sync. If torch.cuda is unavailable, async GPU work (if any) won't be flushed before the timer stops. This is inherent to the design but worth a comment.

📊 Reproducibility

• AUDIT.md references commit SHAs (ca289d6, 452b2c8, 4796a54, f63879b) for branches that are not part of this diff. If those branches are force-pushed or rebased, the audit ledger becomes unverifiable. Pin to tags or include a git log --oneline snapshot.

• optimize_af2 seed parameter is accepted but _select is not shown in the diff. If _select uses random without seeding from seed, protein selection is non-deterministic across runs, undermining A/B reproducibility.

• _fold_all median plddt is None when no predictions emit plddt. The caller (optimize_af2) presumably gates on this, but if the quality check is skipped when plddt is absent, a degraded run (all inference failures) could still produce a throughput-only "result."

💡 Suggestions

• require_timing_partition should return typed fractions with the unattributed key documented in a TypedDict or named tuple rather than a plain dict[str, float]. Callers unpack named keys by string; a typo ("unattributed" vs "unattributed_frac") silently returns 0.0 via .get() in downstream code.

• AUDIT.md claims "1100 passed, 1 skipped" but the diff contains no test files. Add at least a pointer to the test file that covers require_timing_partition's overlap-rejection and the classify_bottleneck → UNCLASSIFIED path, so reviewers can verify the claims without checking out the branch.

• _cmd_profile now returns 1 when not bundle.complete, but bundle.complete is not shown being set when workload.returncode != 0—only bundle.missing is appended. Verify ProfileBundle.complete is a @property derived from missing being empty, otherwise a failed workload still exits 0.

• py-spy subprocess is launched with --pid workload.pid before workload is confirmed running. On very fast-exiting workloads the PID may already be dead when py-spy attaches, producing the "py-spy capture produced no flamegraph" missing-artifact warning with no actionable diagnostic. Consider checking workload.poll() is None before launching py-spy.

🤖 Claude Code Review

Review

🐛 Bugs

require_timing_partition tolerance is too tight for wall-clock aggregates

tolerance = max(1e-12, total * 1e-9)

total * 1e-9 is a relative tolerance of 1 ppb. For a 1-second total, that's 1 ns. Floating-point summation of many per-frame timings will easily exceed this, producing spurious RuntimeError refusals on valid data. A more practical relative tolerance is 1e-6 (1 µs per second of wall time).

_build_stall_phase receives wall-clock but passes per-frame sum to require_timing_partition
In both harness.py files, total is the sum of per-frame _t_total_s, not wall_clock_s. The function signature accepts wall_clock_s but only uses it for the return dict — the partition validation now uses the aggregate frame total instead. If per-frame totals don't sum to wall time (due to parallelism or timing gaps) the partition will refuse correctly or silently eat the discrepancy into unattributed. The intent is ambiguous; document which denominator is authoritative.

classify_bottleneck — sc_score can be None when sc is None, but mem_score >= 1.0 branch falls through correctly, yet the final max(sc_score, mem_score) line would TypeError if sc_score is None and mem_score < 1.0

if max(sc_score, mem_score) < 1.0:   # sc_score could be None here
    return COMPUTE_BOUND

The guard if sc_score is None: return UNCLASSIFIED only fires after this line when mem_score < 1.0. The ordering should be: check sc_score is None before calling max(sc_score, mem_score).

require_positive_work type signature accepts int | float but comparison value <= 0 is falsy for float('-inf') — math.isfinite already covers that, but -inf <= 0 is True so it's fine. No bug, just a note that the isfinite guard does the heavy lifting.

_cmd_edge_manifest empty-manifest check is after print
The success message is printed before the emptiness check, so on failure the user sees both "wrote …: 0 keyframes" and the error. Reorder or gate the success print.

run_profile — sar_output file handle opened with open() directly
If workload.wait() raises (e.g. interrupted), sar_output is never closed and host_procs teardown is skipped. Use a try/finally around the workload.wait() + sampler teardown block.

build_breakdown — data_stall can go negative

data_stall = 1.0 - gpu - sync - cpu

The overlap guard allows gpu + sync + cpu == 1.0 + 1e-9 to pass (it uses > 1.0 + 1e-9), so data_stall can be slightly negative without raising. The old code had max(0.0, ...) for this reason. Either tighten the guard to >= 1.0 or restore the clamp with a comment.

🔒 Security

run_profile — py-spy PID passed to subprocess as str(workload.pid) — safe (integer, no shell=True). No issue.

open(bundle.host_sar, "w") with a user-controlled out_dir — the path derives from CLI args.out_dir. Not a shell injection risk (no shell=True), but an absolute path traversal is possible if the CLI accepts arbitrary paths without sanitization. Low severity in a local benchmarking tool.

⚡ Performance

aggregate — double iteration over gpu_values

gpu_coverage_ok = all(value is not None for value in gpu_values)
gpu_overall = max(value for value in gpu_values if value is not None) if gpu_coverage_ok else None

When gpu_coverage_ok is True, the if value is not None filter iterates again unnecessarily. Minor, but a simple if gpu_coverage_ok: gpu_overall = max(gpu_values) is cleaner.

📊 Reproducibility

optimize_af2 input validation checks warmup >= 0 but warmup=0 means no warm-up runs — this is intentional (the docstring says "warmup" is a count), but it means a caller can silently benchmark cold JIT compilation as if it were steady-state. A minimum of 1 should be considered, or the value should be surfaced in the artifact.

_fold_all does not set a CUDA/torch seed — pLDDT variance across A/B arms from dropout or stochastic ops could affect the quality comparison. If AF2 inference uses dropout, torch.manual_seed should be set per arm.

💡 Suggestions

require_timing_partition does not validate that individual components are ≤ total — a single component larger than the total (e.g. a buggy timer) would be caught by the assigned > total check only if other components are non-negative, which they are by the existing guard. This is fine, but documenting it would help.

AUDIT.md claims 1100 tests pass but is committed to the repo — audit ledgers like this are useful, but living in main they will go stale. Consider converting the ledger to structured JSON/YAML consumed by a test or CI step, so claims about "93 closed findings" can be verified mechanically rather than trusted on faith.

_visible_gpu_count stacklevel=2 in warnings.warn — the function is private and called from _knobs_from_engine_args → _engine_arg_knobs → public callers. stacklevel=2 points to the _visible_gpu_count call site inside _engine_arg_knobs, not to the public caller. This is acceptable for internal tooling but could confuse library users.

AF2ABResult and optimize_af2 — missing math.isfinite check on sph result before returning — require_positive_duration and require_positive_work guard the denominator, but if count is astronomically large the resulting sph could overflow to inf. Add a math.isfinite(sph) assertion before returning.

_cmd_profile now returns 0 if bundle.complete else 1 — bundle.complete is a boolean field whose definition is not shown in the diff. Verify it is set after all bundle.missing.append(...) calls in run_profile, otherwise the completeness check will observe a stale value.

Code Review by Gemini

The pull request introduces a comprehensive audit and hardening of runtime paths, focusing on explicit fallback handling, strict validation, and improved diagnostics. The changes are extensive and consistently apply principles of robustness, preventing missing or malformed evidence from leading to confident but incorrect claims.

Here's a breakdown of the review:

Overall Impression

This is a substantial and well-executed audit. The changes demonstrate a deep understanding of potential failure modes and address them systematically across various components. The introduction of explicit REFUSE/FLAG/WARN states, detailed diagnostics, and stricter input validation significantly improves the trustworthiness and debuggability of the system. The refactoring of graph prediction and config parsing, especially for sparse MoE models, is a critical improvement.

Specific Issues and Suggestions

AUDIT.md

• Clarity on "Wiring gaps confirmed": The summary states "Wiring gaps confirmed: 12". While the ledger lists 93 fixed items, it's not immediately clear if these 12 are a subset of the 93, or a different category. Given the "Status: complete", it implies they were also addressed.
  • Suggestion: Clarify the relationship between "Fallback masks and wiring failures closed: 93" and "Wiring gaps confirmed: 12" in the executive summary. For example, "12 categories of wiring gaps were identified and closed across 93 specific instances."

benchmarks/biotech/optimize.py

• Missing math import for math.isfinite: The math module is used for math.isfinite but not explicitly imported in this file.
  • Suggestion: Add import math at the top of the file.

--- a/benchmarks/biotech/optimize.py
+++ b/benchmarks/biotech/optimize.py
@@ -18,6 +18,7 @@
 
 import argparse
 import json
+import math
 import os
 import statistics
 import time
 from dataclasses import dataclass

gitm/agents/autoresearch.py

• _engine_arg_knobs can return an empty list without warning: If _knobs_from_engine_args returns an empty list, it falls back to _FALLBACK_KNOBS. However, if _knobs_from_engine_args itself fails (e.g., due to an unexpected EngineArgs structure), it catches the exception and returns list(_FALLBACK_KNOBS) without a warning. This could hide issues with introspection.
  • Suggestion: Add a warning if _knobs_from_engine_args returns an empty list, similar to how it warns about introspection failures.

--- a/gitm/agents/autoresearch.py
+++ b/gitm/agents/autoresearch.py
@@ -661,14 +673,21 @@
         return list(_FALLBACK_KNOBS)
     try:
         knobs = _knobs_from_engine_args(EngineArgs, gpu_count=gpu_count)
         if knobs:
             return knobs
         warnings.warn(
             "vLLM EngineArgs introspection produced no searchable knobs; autoresearch "
             "is using the frozen fallback knob catalog",
             RuntimeWarning,
             stacklevel=2,
         )
         return list(_FALLBACK_KNOBS)
     except Exception as exc:
         warnings.warn(
             "vLLM EngineArgs introspection failed; autoresearch is using the frozen "
             f"fallback knob catalog ({type(exc).__name__}: {exc})",
             RuntimeWarning,
             stacklevel=2,
         )
         return list(_FALLBACK_KNOBS)

gitm/cli.py

• _parse_target validation for target: The ValueError message for _parse_target is generic. It could be more specific to indicate the expected range.
  • Suggestion: Make the error message more specific, e.g., "target floor must be finite and in (0, 1], got {s!r}".

--- a/gitm/cli.py
+++ b/gitm/cli.py
@@ -141,10 +142,11 @@
     )
     attach.add_argument(
         "--dry-run",
         action="store_true",
         help="Plan the attach without touching the live process.",
     )
 
     _add_capture(sub)
 
     sub.add_parser("doctor", help="Probe environment, GPUs, and data locations.")
@@ -199,7 +201,7 @@
     if s.endswith("%"):
         return float(s[:-1]) / 100.0
     else:
         target = float(s)
     if not math.isfinite(target) or not 0.0 < target <= 1.0:
         raise ValueError(f"target floor must be finite and in (0, 1], got {s!r}")
     return target

gitm/importers/analyze.py

• _artifact_stem could produce empty string: If value is an empty string or contains only characters that are replaced by _, stem could be empty. strip("._") would then return an empty string. This could lead to issues when used as a filename.
  • Suggestion: Add a fallback to a default name like "trace" if the sanitized stem is empty.

--- a/gitm/importers/analyze.py
+++ b/gitm/importers/analyze.py
@@ -154,6 +154,7 @@
 def _artifact_stem(value: str) -> str:
     """Return a portable filename stem without changing the report identifier."""
     stem = re.sub(r"[^A-Za-z0-9_.-]+", "_", value).strip("._")
-    return stem
+    return stem or "trace"

gitm/optimizer/apply.py

• LiveEngineApplicator._bench_stats error message: The error message for invalid throughput samples is generic. It could be more specific about which sample was invalid.
  • Suggestion: Include the problematic sample value in the error message.

--- a/gitm/optimizer/apply.py
+++ b/gitm/optimizer/apply.py
@@ -350,7 +350,7 @@
         if invalid:
             raise ValueError(
                 "decode throughput samples must be finite and positive; "
-                f"observed {invalid!r}"
+                f"observed {invalid}"
             )
         mean = sum(samples) / len(samples)
         if len(samples) < 2:
             return mean, 0.0

gitm/planner/graph.py

• predict_graph kv_heads_rank calculation: The kv_heads_rank calculation has a potential bug. If model.num_kv_heads is 0 (e.g., for a model without KV heads or a dense model where it defaults to n_heads but n_heads is 0), and tp is also 0, it would lead to a ZeroDivisionError. While tp is validated to be positive, model.num_kv_heads can be 0.
  • Suggestion: Add a check for model.num_kv_heads > 0 before performing division by tp in the kv_heads_rank calculation. If model.num_kv_heads is 0, kv_heads_rank should also be 0.

--- a/gitm/planner/graph.py
+++ b/gitm/planner/graph.py
@@ -220,13 +220,16 @@
     if sharding.ep != 1:
         raise ValueError("dense graph does not support expert parallelism")
     tp = sharding.tp
     if model.n_heads % tp:
         raise ValueError(f"n_heads={model.n_heads} must be divisible by tp={tp}")
-    if model.num_kv_heads >= tp:
+    if model.num_kv_heads == 0:
+        kv_heads_rank = 0
+    elif model.num_kv_heads >= tp:
         if model.num_kv_heads % tp:
             raise ValueError(
                 f"num_kv_heads={model.num_kv_heads} must be divisible by tp={tp}"
             )
         kv_heads_rank = model.num_kv_heads / tp
     else:
         if tp % model.num_kv_heads:
             raise ValueError(

gitm/planner/moe_graph.py

• validate_sparse_moe_config quantization_config check: The check q.get("quant_method") is not None and weight_bytes_is_fallback(str(q["quant_method"])) assumes q is a dictionary. If q is not a dictionary (e.g., None or a string), q["quant_method"] would raise an error. The preceding check if q is not None and not isinstance(q, dict): handles this, but the subsequent q = {} is inside the elif block, meaning q might still not be a dict if the if condition was met.
  • Suggestion: Move q = {} outside the elif block, or ensure q is a dict before accessing q["quant_method"].

--- a/gitm/planner/moe_graph.py
+++ b/gitm/planner/moe_graph.py
@@ -90,10 +90,11 @@
     elif any(isinstance(r, bool) or not isinstance(r, int) or r < 0 for r in ratios):
         errors.append("compress_ratios must contain non-negative integers")
     q = cfg.get("quantization_config")
     if q is not None and not isinstance(q, dict):
         errors.append("quantization_config must be an object when declared")
-    elif isinstance(q, dict) and q.get("quant_method") is not None and weight_bytes_is_fallback(str(q["quant_method"])):
+        q = {} # Ensure q is a dict for subsequent checks
+    if isinstance(q, dict) and q.get("quant_method") is not None and weight_bytes_is_fallback(str(q["quant_method"])):
         errors.append(f"quantization_config.quant_method={q['quant_method']!r} is not priceable")
     for key in ("expert_dtype", "torch_dtype"):
         value = cfg.get(key)
         if value is None:

gitm/scheduler/loop.py

• _engine_throughput_fn toks initialization: toks is initialized to None and then checked if it's None. If it remains None, an error is raised. However, the toks: float = 1.0 line is commented out, which was likely the intended default. If no key is found, toks remains None.
  • Suggestion: Initialize toks to 1.0 if that's the desired fallback for "no tokens produced" (as implied by the original code comment), or ensure the error message is clear that no positive work was found. Given the require_positive_work call, 1.0 is a valid default.

--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -300,7 +300,7 @@
         dt = require_positive_duration(
             time.perf_counter() - t0, context="live-engine throughput probe"
         )
         # First key that is actually present wins — `or` would treat a legitimate
         # 0 (a window that produced no tokens) as missing and fabricate a count.
-        toks: float = 1.0
+        toks: float | None = None
         if isinstance(out, dict):
             for key in ("generated_tokens", "decode_steps", "events"):
                 if out.get(key) is not None:
                     toks = float(out[key])
                     break

• _execution_graph compatibility path for hf: The compatibility path for _execution_graph (when pctx is HardwareSpec and sched is BatchConfig) has a hardcoded sparse_cfg dictionary. This dictionary uses cfg.get("key", default_value) for many fields. If cfg is missing a key, it will use the default. However, spec_from_hf_config (which is called next) now performs strict validation via validate_sparse_moe_config. This means the compatibility path might construct a sparse_cfg that spec_from_hf_config will then reject, leading to a ValueError instead of a graceful fallback.
  • Suggestion: The compatibility path should either perform the same strict validation as the main path or ensure that the sparse_cfg it constructs is always valid for spec_from_hf_config. Given that this is a compatibility path, it might be acceptable to be less strict and let spec_from_hf_config handle the defaults, but the current setup could lead to unexpected errors. For now, I'll suggest ensuring the sparse_cfg is valid.

--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -350,29 +350,30 @@
     if isinstance(pctx, HardwareSpec) and isinstance(sched, BatchConfig):
         hf, _ = _engine_hf_config(engine)
         if hf is None:
             return predict_graph(model=ModelSpec(), hw=pctx, batch=sched), False
         cfg = _hf_config_dict(hf)
         if is_sparse_moe_graph_config(cfg):
-            sparse_cfg = {
-                "hidden_size": cfg.get("hidden_size", 4096),
-                "num_hidden_layers": cfg.get("num_hidden_layers", 43),
-                "num_attention_heads": cfg.get("num_attention_heads", 64),
-                "num_key_value_heads": cfg.get("num_key_value_heads", 1),
-                "head_dim": cfg.get("head_dim", 512),
-                "qk_rope_head_dim": cfg.get("qk_rope_head_dim", 64),
-                "q_lora_rank": cfg.get("q_lora_rank", 1024),
-                "o_lora_rank": cfg.get("o_lora_rank", 1024),
-                "o_groups": cfg.get("o_groups", 8),
-                "vocab_size": cfg.get("vocab_size", 129280),
-                "n_routed_experts": cfg.get("n_routed_experts", 256),
-                "n_shared_experts": cfg.get("n_shared_experts", 1),
-                "num_experts_per_tok": cfg.get("num_experts_per_tok", 6),
-                "moe_intermediate_size": cfg.get("moe_intermediate_size", 2048),
-                "index_n_heads": cfg.get("index_n_heads", 64),
-                "index_head_dim": cfg.get("index_head_dim", 128),
-                "index_topk": cfg.get("index_topk", 512),
-                "sliding_window": cfg.get("sliding_window", 128),
-                "compress_ratios": cfg.get("compress_ratios", [0] * 43),
-                "torch_dtype": cfg.get("torch_dtype", "bfloat16"),
-                "expert_dtype": cfg.get("expert_dtype", "fp4"),
-                "quantization_config": cfg.get("quantization_config", {"quant_method": "fp8"}),
-                **{k: v for k, v in cfg.items() if k in {"hc_mult", "hc_sinkhorn_iters", "num_hash_layers", "num_nextn_predict_layers", "dspark_target_layer_ids", "dspark_markov_rank"}},
-            }
+            # The compatibility path is deliberately tolerant of older HF fixtures that
+            # omit torch_dtype; the production path remains strict.
+            # This block ensures that the sparse_cfg is valid for spec_from_hf_config
+            # by providing defaults for all required fields if they are missing.
+            sparse_cfg = {k: v for k, v in cfg.items()}
+            sparse_cfg.setdefault("hidden_size", 4096)
+            sparse_cfg.setdefault("num_hidden_layers", 43)
+            sparse_cfg.setdefault("num_attention_heads", 64)
+            sparse_cfg.setdefault("num_key_value_heads", 1)
+            sparse_cfg.setdefault("head_dim", 512)
+            sparse_cfg.setdefault("qk_rope_head_dim", 64)
+            sparse_cfg.setdefault("q_lora_rank", 1024)
+            sparse_cfg.setdefault("o_lora_rank", 1024)
+            sparse_cfg.setdefault("o_groups", 8)
+            sparse_cfg.setdefault("vocab_size", 129280)
+            sparse_cfg.setdefault("n_routed_experts", 256)
+            sparse_cfg.setdefault("n_shared_experts", 1)
+            sparse_cfg.setdefault("num_experts_per_tok", 6)
+            sparse_cfg.setdefault("moe_intermediate_size", 2048)
+            sparse_cfg.setdefault("index_n_heads", 64)
+            sparse_cfg.setdefault("index_head_dim", 128)
+            sparse_cfg.setdefault("index_topk", 512)
+            sparse_cfg.setdefault("sliding_window", 128)
+            sparse_cfg.setdefault("compress_ratios", [0] * sparse_cfg["num_hidden_layers"])
+            sparse_cfg.setdefault("torch_dtype", "bfloat16")
+            sparse_cfg.setdefault("expert_dtype", "fp4")
+            sparse_cfg.setdefault("quantization_config", {"quant_method": "fp8"})
+            # Ensure other optional fields are also present if they exist in cfg
+            for k in {"hc_mult", "hc_sinkhorn_iters", "num_hash_layers", "num_nextn_predict_layers", "dspark_target_layer_ids", "dspark_markov_rank"}:
+                if k in cfg:
+                    sparse_cfg.setdefault(k, cfg[k])
+
             spec = spec_from_hf_config(sparse_cfg, name=str(cfg.get("model_type") or "sparse-moe"))
             return predict_moe_graph(spec, pctx, sched, ShardingConfig()), True
         # The direct compatibility path is deliberately tolerant of older HF
         # fixtures that omit torch_dtype; the production path remains strict.
         dense_cfg = dict(cfg)
         dense_cfg.setdefault("torch_dtype", "bf16")
         spec, error = _dense_spec_from_config(dense_cfg)
         if spec is None:
             raise ValueError(error)
         return predict_graph(model=spec, hw=pctx, batch=sched, sharding=ShardingConfig()), False

gitm/serve/attach.py

• _emit_predicted_graph kv_bytes_per_token_per_sequence: The kv_bytes_per_token_per_sequence is set to 0.0 in the payload when kv_bytes_per_token(spec) returns None. This happens if spec.kv_latent_dim is 0. While this might be a valid scenario, it's worth noting that 0.0 might be misleading if the value is truly "unavailable" rather than "zero".
  • Suggestion: Consider using None for kv_bytes_per_token_per_sequence in the payload if g.kv_bytes_per_token_per_sequence is None, to distinguish between "zero" and "unavailable".

--- a/gitm/serve/attach.py
+++ b/gitm/serve/attach.py
@@ -400,7 +400,9 @@
                 ),
                 "kv_bytes_per_token_per_sequence": g.kv_bytes_per_token_per_sequence,
                 "kv_fixed_bytes_per_sequence": g.kv_fixed_bytes_per_sequence,
                 "has_unpriced_collectives": g.has_unpriced_collectives,
                 "has_unpriced_nodes": g.has_unpriced_nodes,
                 "has_unpriced_compute": g.has_unpriced_compute,
                 "has_unpriced_memory": g.has_unpriced_memory,
                 "has_fallback_peaks": g.has_fallback_peaks,

gitm/serve/model_config.py

• validate_moe_config qk_rope_head_dim and n_shared_experts validation: The validation for qk_rope_head_dim and n_shared_experts checks for isinstance(value, bool) or not isinstance(value, int) or value < 0. This is correct for non-negative integers. However, the error message states "must be a declared non-negative integer". If the value is None, it would fall into this check.
  • Suggestion: Add a specific check for value is None first, and then the type/range check, to provide a more precise error message if the field is simply missing.

--- a/gitm/serve/model_config.py
+++ b/gitm/serve/model_config.py
@@ -230,14 +230,20 @@
         if isinstance(value, bool) or not isinstance(value, int) or value <= 0:
             missing.append(f"{key} must be a declared positive integer, got {value!r}")
     qk_rope = cfg.get("qk_rope_head_dim")
-    if isinstance(qk_rope, bool) or not isinstance(qk_rope, int) or qk_rope < 0:
+    if qk_rope is None:
+        missing.append("qk_rope_head_dim must be declared; cannot be guessed")
+    elif isinstance(qk_rope, bool) or not isinstance(qk_rope, int) or qk_rope < 0:
         missing.append(
             f"qk_rope_head_dim must be a declared non-negative integer, got {qk_rope!r}"
         )
     shared = cfg.get("n_shared_experts")
-    if isinstance(shared, bool) or not isinstance(shared, int) or shared < 0:
+    if shared is None:
+        missing.append("n_shared_experts must be declared; cannot be guessed")
+    elif isinstance(shared, bool) or not isinstance(shared, int) or shared < 0:
         missing.append(
             f"n_shared_experts must be a declared non-negative integer, got {shared!r}"
         )
     if cfg.get("torch_dtype") is None:
         missing.append("torch_dtype must be declared; activation width cannot be guessed")

gitm/tracer/vllm_stats.py

• _v1_scheduler_stats num_running and num_waiting handling: The code iterates through _v1_scheduler_stats twice in read_scheduler_stats. The second loop is redundant and could potentially overwrite diagnostics if the first loop already found a value.
  • Suggestion: Remove the second loop in read_scheduler_stats that calls _v1_scheduler_stats.

--- a/gitm/tracer/vllm_stats.py
+++ b/gitm/tracer/vllm_stats.py
@@ -366,14 +366,6 @@
             sample.gpu_cache_usage = usage
             saw_any = True
 
-        # vLLM V1: fill running / waiting / cache from the scheduler's stat object
-        # where the VO deques weren't exposed (they read empty on V1)
-        for sch in schedulers:
-            for field_name, val in _v1_scheduler_stats(sch).items():
-                if getattr(sample, field_name) is None:
-                    setattr(sample, field_name, val)
-                    saw_any = True
-
     # Total unfinished — a stable public method on LLMEngine across versions.
     getter = _first_attr(
         engine,
         "get_num_unfinished_requests",

Conclusion

The changes are well-aligned with the commit message and significantly improve the robustness and diagnostic capabilities of the system. The identified issues are minor and mostly concern error message clarity or small logical redundancies, which do not detract from the overall quality of this substantial refactor.

🤖 Claude Code Review

Review Feedback

🐛 Bugs

require_timing_partition tolerance is far too tight

tolerance = max(1e-12, total * 1e-9)

1e-9 relative tolerance is sub-nanosecond for typical wall-clock totals (e.g., 10s → 10ns tolerance). Floating-point accumulation across many sum() calls on per-frame timings will routinely exceed this, causing spurious RuntimeError on valid data. Use 1e-6 or math.isclose defaults (rel_tol=1e-9 is the standard but 1e-6 is safer here).

_visible_gpu_count stacklevel is wrong

warnings.warn(..., stacklevel=2)

_visible_gpu_count is a module-private helper; the public caller is _engine_arg_knobs or _knobs_from_engine_args. stacklevel=2 points at the internal call site, not at the user code. Should be 3 or the warning should be issued at the call site.

classify_bottleneck — sc_score vs mem_score comparison unsafe

if mem_score >= 1.0 and (sc_score is None or mem_score > sc_score):
    return MEMORY_BOUND
if sc_score is None:
    return UNCLASSIFIED
if max(sc_score, mem_score) < 1.0:
    return COMPUTE_BOUND
return IDLE_STALL if sc_score >= mem_score else MEMORY_BOUND

If sc_score is None and mem_score < 1.0, the function reaches the final return IDLE_STALL branch — but sc_score is None there. The if sc_score is None: return UNCLASSIFIED guard only fires when mem_score < 1.0. The max(sc_score, mem_score) call on the next line would raise TypeError. This path is reachable when _serialized_fraction returns None and memory pressure is moderate.

sanity.py — division semantics changed
Previously cold["_t_total_s"] / max(warm["_t_total_s"], 1e-9) computed a ratio even if cold was zero. Now require_positive_duration is applied to both values, so a zero cold time (e.g., cached runner returning instantly) raises instead of producing a suspicious but informative 0.0x speedup. The old behaviour was arguably more useful for debugging.

_cmd_edge_manifest empty-check is post-write
The not rows guard is checked after write_manifest(args.out, rows) has already written an empty file. The error message says "not usable" but the empty file exists on disk. Check before writing.

run_profile — sar_output file handle never closed on exception

sar_output = open(bundle.host_sar, "w")

If subprocess.Popen raises after sar_output is opened, the file handle leaks. Should use a context manager or try/finally.

aggregate — partial GPU coverage silently passes the gate

gpu_coverage_ok = all(value is not None for value in gpu_values)
gpu_overall = max(...) if gpu_coverage_ok else None

If only some runs lack telemetry gpu_coverage_ok is False and gpu_overall becomes None, failing the gate — correct. But the max(value for value in gpu_values if value is not None) expression inside the ternary is evaluated unconditionally (Python evaluates both sides lazily only in if/else expressions but the filter-generator is fine). This is actually correct but the comment if gpu_coverage_ok makes the filtered max dead code. Simplify to avoid confusion.

🔒 Security

run_profile — workload.pid passed to py-spy via str(...)

"--pid", str(workload.pid),

workload.pid is an integer from subprocess.Popen so there is no injection risk here, but if tools.py_spy or tools.sar were ever sourced from user-controlled config without validation, the list-form Popen call would still be safe. No issue — just worth noting the list form is correct.

⚡ Performance

aggregate iterates runs twice unnecessarily
The provenance loop and the gpu_values comprehension are separate passes. Minor, but could be combined.

_build_stall_phase calls sum() four times over timings
Each call is a full iteration. A single pass accumulating all four sums would be cleaner and avoids repeated attribute lookups for large frame counts.

📊 Reproducibility

AUDIT.md references specific commit hashes as stable anchors
Commits ca289d6, 452b2c8, 4796a54, f63879b are hardcoded in the document. If any branch is force-pushed or the repo is rebased, these references silently become wrong without any runtime check. Consider using tag names instead of raw SHAs for long-lived audit documents.

optimize_af2 — seed parameter is accepted but not forwarded to PyTorch/CUDA RNG

def optimize_af2(stage, seed, *, n_proteins, max_len, warmup, plddt_tol):

The new input validation guards n_proteins, max_len, warmup, plddt_tol but seed has no validation and — based on the visible diff — is never passed to torch.manual_seed or random.seed. If the caller relies on this for reproducibility the seed is silently ignored.

💡 Suggestions

require_timing_partition — unattributed key can collide with component names
If a caller passes components_s={"unattributed": 1.0, ...}, the key is overwritten silently. Add a guard:

if "unattributed" in components_s:
    raise ValueError("'unattributed' is a reserved partition key")

require_positive_work — type annotation accepts float but the check uses <= 0
Non-integer floats like 0.5 would pass. If the intent is genuinely integer work counts (frames, tokens), restrict the type and add an isinstance check analogous to the optimize_af2 guards added for n_proteins.

AUDIT.md finding ledger has no machine-readable format
93 findings tracked in a Markdown table make programmatic regression checking (e.g., "has finding #N been re-opened?") manual. A companion findings.json or YAML would allow CI to assert all statuses remain fixed.

_cmd_profile — bundle.complete semantics undocumented
The return value now changes from 0 to 1 when bundle.complete is falsy, but there is no visible definition of what sets bundle.complete = False in the diff. Reviewers cannot verify the gate without seeing ProfileBundle.complete's setter logic.

Missing __all__ export for require_timing_partition and require_positive_work
gitm/_timing.py has no __all__. Wildcard imports from the module would expose the implementation unexpectedly. Add __all__ for the three public predicates.